README.md: adding a git style guide by vbatts · Pull Request #168 · opencontainers/runtime-spec

vbatts · 2015-09-09T22:55:34Z

pulled from http://chris.beams.io/posts/git-commit/

Signed-off-by: Vincent Batts vbatts@hashbangbash.com

wking · 2015-09-09T23:08:08Z

On Wed, Sep 09, 2015 at 03:55:36PM -0700, Vincent Batts wrote:

pulled from http://chris.beams.io/posts/git-commit/

Maybe link there for folks who want more detail on or the motivation
behind a particular guideline?

wking · 2015-09-09T23:08:47Z

README.md

50 seems overly strict, especially if we want a prefix. Looking through our 1113 non-merge commits (with --format=%s) we only have 62% ≤50 chars, and but 78% are ≤ 70 chars. Of course some of those are because folks forgot the blank-line after the summary. But I still think a larger limit here would help encourage more explicit summaries.

this is actually from git-commit(1) own guidelines. https://www.kernel.org/pub/software/scm/git/docs/git-commit.html

Though not required, it’s a good idea to begin the commit message with a single short (less than 50 character) line summarizing the change, followed by a blank line and then a more thorough description. The text up to the first blank line in a commit message is treated as the commit title, and that title is used throughout Git.

On Wed, Sep 09, 2015 at 04:12:24PM -0700, Vincent Batts wrote:

+2. Limit the subject line to 50 characters

this is actually from git-commit(1) own guidelines.

Then I think everyone is just blindly copying it around ;). Looking
at the git.git history through v2.3.4 (git log --no-merges --format=%s
v2.3.4), we have 29853 commits, with 56% ≤ 50 chars and 94% ≤ 70
chars.

And because it's been too long since I've busted out some Matplotlib, here's a histogram for git.git:

Generated with:

import sys import matplotlib.pyplot lines = sys.stdin.read().splitlines() lengths = [len(line) for line in lines] figure = matplotlib.pyplot.figure() axes = figure.add_subplot(1, 1, 1) axes.hist(lengths, 50) axes.set_xlabel('commit message length (chars)') axes.set_ylabel('counts in git.git through v2.3.4') figure.savefig('x.png')

;).

Jesus Trevor... I ....
On Sep 9, 2015 19:37, "W. Trevor King" notifications@github.com wrote:

In README.md
#168 (comment):

@@ -167,4 +169,16 @@ using your real name (sorry, no pseudonyms or anonymous contributions.)

You can add the sign off when creating the git commit via git commit -s.

+### Commit Style
+
+Simple house-keeping for clean git history.
+
+1. Separate subject from body with a blank line
+2. Limit the subject line to 50 characters

And because it's been too long since I've busted out some Matplotlib,
here's a histogram for git.git:

[image: x]
https://cloud.githubusercontent.com/assets/209920/9776781/d9e8bb6a-5710-11e5-9fd1-a7b62e54b677.png

Generated with:

import sysimport matplotlib.pyplot
lines = sys.stdin.read().splitlines()
lengths = [len(line) for line in lines]
figure = matplotlib.pyplot.figure()
axes = figure.add_subplot(1, 1, 1)
axes.hist(lengths, 50)
axes.set_xlabel('commit message length (chars)')
axes.set_ylabel('counts in git.git through v2.3.4')
figure.savefig('x.png')

;).

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/168/files#r39110526.

Regardless of the past commits, this would obviously be for future commits.
On Sep 9, 2015 19:38, "Vincent Batts" vbatts@hashbangbash.com wrote:

Jesus Trevor... I ....
On Sep 9, 2015 19:37, "W. Trevor King" notifications@github.com wrote:

In README.md
#168 (comment):

@@ -167,4 +169,16 @@ using your real name (sorry, no pseudonyms or anonymous contributions.)

You can add the sign off when creating the git commit via git commit -s.

+### Commit Style
+
+Simple house-keeping for clean git history.
+
+1. Separate subject from body with a blank line
+2. Limit the subject line to 50 characters

And because it's been too long since I've busted out some Matplotlib,
here's a histogram for git.git:

[image: x]
https://cloud.githubusercontent.com/assets/209920/9776781/d9e8bb6a-5710-11e5-9fd1-a7b62e54b677.png

Generated with:

import sysimport matplotlib.pyplot
lines = sys.stdin.read().splitlines()
lengths = [len(line) for line in lines]
figure = matplotlib.pyplot.figure()
axes = figure.add_subplot(1, 1, 1)
axes.hist(lengths, 50)
axes.set_xlabel('commit message length (chars)')
axes.set_ylabel('counts in git.git through v2.3.4')
figure.savefig('x.png')

;).

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/168/files#r39110526.

On Wed, Sep 09, 2015 at 04:38:49PM -0700, Vincent Batts wrote:

+2. Limit the subject line to 50 characters

Jesus Trevor... I ....

Graphs help people make decisions, right? ;)

Or help give graphics for a slide deck...
On Sep 9, 2015 19:42, "W. Trevor King" notifications@github.com wrote:

In README.md
#168 (comment):

@@ -167,4 +169,16 @@ using your real name (sorry, no pseudonyms or anonymous contributions.)

You can add the sign off when creating the git commit via git commit -s.

+### Commit Style
+
+Simple house-keeping for clean git history.
+
+1. Separate subject from body with a blank line
+2. Limit the subject line to 50 characters

On Wed, Sep 09, 2015 at 04:38:49PM -0700, Vincent Batts wrote: > +2. Limit
the subject line to 50 characters Jesus Trevor... I ....
Graphs help people make decisions, right? ;)

—
Reply to this email directly or view it on GitHub
https://github.com/opencontainers/specs/pull/168/files#r39110896.

On Wed, Sep 09, 2015 at 04:41:54PM -0700, Vincent Batts wrote:

+2. Limit the subject line to 50 characters

Regardless of the past commits, this would obviously be for future
commits.

This limit has been part of the Git docs for a while now, and they're
still not keeping under 50 (they also reference the 50-char soft limit
in Documentation/SubmittingPatches). I'd prefer just putting a
hard-limit at 70 chars in your validation tool (#167), but I'm ok
putting a limit of 50 in that validation tool (and still merging if
"commit summary is too long" is the only error). I just think wedging
useful summaries into 50-len(prefix) is going to be more trouble than
its worth.

vbatts · 2015-09-09T23:08:55Z

particular guideline?

wking · 2015-09-09T23:14:46Z

On Wed, Sep 09, 2015 at 03:55:36PM -0700, Vincent Batts wrote:

pulled from http://chris.beams.io/posts/git-commit/

And you seem to have left out what I think is the most important point
in Chris' post: what to put in the body 1. Was there a reason to
leave that out?

vbatts · 2015-09-09T23:17:42Z

Not a strong compulsion. I wonder if there is a better phrasing for that
bullet point.
On Sep 9, 2015 7:14 PM, "W. Trevor King" notifications@github.com wrote:

On Wed, Sep 09, 2015 at 03:55:36PM -0700, Vincent Batts wrote:

pulled from http://chris.beams.io/posts/git-commit/

And you seem to have left out what I think is the most important point
in Chris' post: what to put in the body 1. Was there a reason to
leave that out?

—
Reply to this email directly or view it on GitHub
#168 (comment).

wking · 2015-09-09T23:20:56Z

On Wed, Sep 09, 2015 at 04:08:56PM -0700, Vincent Batts wrote:

particular guideline?

In Chris' post, each bullet point links to a section explaining why
and how. The list your bringing in here is basically a table of
contents that references that more detailed information.

wking · 2015-09-09T23:22:57Z

On Wed, Sep 09, 2015 at 04:17:44PM -0700, Vincent Batts wrote:

Not a strong compulsion. I wonder if there is a better phrasing for
that bullet point.

I'm fairly comfortable with his phrasing. I usually talk about
“motivation” when I'm talking folks through good commit hygiene, but
his “why” is pretty much the same, and it's easier to type ;).

vbatts · 2015-09-10T03:07:30Z

updated

vbatts · 2015-09-10T03:08:28Z

And we can talk validation once the guide is in place. I'm not opposed to len() < 70

wking · 2015-09-10T03:29:50Z

README.md

The other entries are phrased to match an implicit “You should …”. For example, “You should separate…”. So maybe:

Prefix the subject (when possible) with a keyword scoping the change (e.g. “README: …”, “runtime: …”)

And maybe shift this up to sit with the other subject-related entries from earlier in the list.

wking · 2015-09-10T03:36:44Z

On Wed, Sep 09, 2015 at 08:08:29PM -0700, Vincent Batts wrote:

And we can talk validation once the guide is in place. I'm not
opposed to len() < 70

Ok.

I left some comments about minor nits with 3cbd157, but I don't think
it's anything that's worth holding up a merge here (i.e. I'm happy to
file nit-fix PRs after this lands if we don't want to finish polishing
all of that out here).

laijs · 2015-09-10T13:33:54Z

After I have read so many barren changelog from the docker.git, it is important that the following statement be included.

All the important/useful/essential information on the conversation of the PR should be copied to the changelog.

This statement makes 7. Use the body to explain what and why vs. how be taken into effect. The convenience of the GitHub entice every body write dryasdust changelogs.

vbatts · 2015-09-10T13:34:50Z

@laijs is a link to the conversation sufficient?

laijs · 2015-09-10T13:40:01Z

The link is inconvenient when git log to see what has been changed recently and why,
when git blame to find something, or when there is no networking.

I'm a 8years of linux-kernel developer of which community is highly value the changelog.

wking · 2015-09-10T16:42:18Z

On Thu, Sep 10, 2015 at 06:33:56AM -0700, Lai Jiangshan wrote:

All the important/useful/essential information on the conversation
of the PR should be copied to the changelog.

This is my personal preference too, but if we're documenting good
commit messages at the level of “remember to put a blank line between
the subject and the body”, I think it might be reaching too far for
now ;). Once we no longer need to talk about syntax, we can go back
with another pass and try to work on getting conversation summaries in
the commit message.

vbatts · 2015-09-10T17:49:33Z

@laijs PTAL

laijs · 2015-09-11T00:41:50Z

The change in the *.md looks good to me.

The rule 3/5 should apply for the sentence after the keyword of the subject,
so I think the subject of this change could be "README.md: Add a git style guide"

vbatts · 2015-09-11T01:21:48Z

@laijs good point. :-)
done

laijs · 2015-09-11T02:19:23Z

LGTM

wking · 2015-09-11T04:14:39Z

On Thu, Sep 10, 2015 at 05:41:52PM -0700, Lai Jiangshan wrote:

The rule 3/5 should apply for the sentence after the keyword of the
subject…

Other projects don't do this (e.g. git.git suggests lowecase after the
prefix 1), but I agree that “Capitalize the subject line” with the
“runtime: ...” example makes it pretty clear that it's the post-prefix
bit that gets Sentence cased.

… so I think the subject of this change could be "README.md: Add a
git style guide"

And I think this matches the style we're recommending.

vbatts · 2015-09-11T11:15:56Z

On Sep 11, 2015 12:14 AM, "W. Trevor King" notifications@github.com wrote:

On Thu, Sep 10, 2015 at 05:41:52PM -0700, Lai Jiangshan wrote:

The rule 3/5 should apply for the sentence after the keyword of the
subject…

Other projects don't do this (e.g. git.git suggests lowecase after the
prefix 1), but I agree that “Capitalize the subject line” with the
“runtime: ...” example makes it pretty clear that it's the post-prefix
bit that gets Sentence cased.

… so I think the subject of this change could be "README.md: Add a
git style guide"

And I think this matches the style we're recommending.

What are you saying? Yes or no to capitalization?

wking · 2015-09-11T14:05:40Z

On Fri, Sep 11, 2015 at 04:15:57AM -0700, Vincent Batts wrote:

What are you saying? Yes or no to capitalization?

I'm agnostic on capitalization, but with the current capitalization
rule, I think your 142c257 looks good. I was mostly saying that I
thought the current rule phrasing seemed clear enough to me, thanks to
the “runtime: ...” example ruling out the “capitalization means the
prefix” interpretation.

vbatts · 2015-09-11T14:27:09Z

k

On Fri, Sep 11, 2015 at 10:05 AM, W. Trevor King notifications@github.com
wrote:

On Fri, Sep 11, 2015 at 04:15:57AM -0700, Vincent Batts wrote:

What are you saying? Yes or no to capitalization?

I'm agnostic on capitalization, but with the current capitalization
rule, I think your 142c257 looks good. I was mostly saying that I
thought the current rule phrasing seemed clear enough to me, thanks to
the “runtime: ...” example ruling out the “capitalization means the
prefix” interpretation.

—
Reply to this email directly or view it on GitHub
#168 (comment).

crosbymichael · 2015-09-11T19:13:21Z

LGTM

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>

mrunalp · 2015-09-11T20:26:22Z

LGTM

README.md: adding a git style guide

wking reviewed Sep 9, 2015
View reviewed changes

vbatts force-pushed the git-style-guide branch from efd907f to 3cbd157 Compare September 10, 2015 03:05

wking reviewed Sep 10, 2015
View reviewed changes

vbatts force-pushed the git-style-guide branch from 3cbd157 to 05d546a Compare September 10, 2015 17:49

vbatts force-pushed the git-style-guide branch from 05d546a to 142c257 Compare September 11, 2015 01:21

README.md: Add a git commit style guide

82835aa

Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>

vbatts force-pushed the git-style-guide branch from 142c257 to 82835aa Compare September 11, 2015 20:24

mrunalp pushed a commit that referenced this pull request Sep 11, 2015

Merge pull request #168 from vbatts/git-style-guide

f6ec7a7

README.md: adding a git style guide

mrunalp merged commit f6ec7a7 into opencontainers:master Sep 11, 2015

vbatts deleted the git-style-guide branch September 14, 2015 14:28

wking mentioned this pull request May 3, 2016

Update contributing guidelines and add maintainer guidelines opencontainers/runtime-tools#56

Merged

wking mentioned this pull request May 19, 2016

CONTRIBUTING: Don't specify a 50-char limit opencontainers/project-template#10

Merged

Conversation

vbatts commented Sep 9, 2015

Uh oh!

wking commented Sep 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbatts commented Sep 9, 2015

Uh oh!

wking commented Sep 9, 2015

Uh oh!

vbatts commented Sep 9, 2015

Uh oh!

wking commented Sep 9, 2015

Uh oh!

wking commented Sep 9, 2015

Uh oh!

vbatts commented Sep 10, 2015

Uh oh!

vbatts commented Sep 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Sep 10, 2015

Uh oh!

laijs commented Sep 10, 2015

Uh oh!

vbatts commented Sep 10, 2015

Uh oh!

laijs commented Sep 10, 2015

Uh oh!

wking commented Sep 10, 2015

Uh oh!

vbatts commented Sep 10, 2015

Uh oh!

laijs commented Sep 11, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

laijs commented Sep 11, 2015

Uh oh!

wking commented Sep 11, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

wking commented Sep 11, 2015

Uh oh!

vbatts commented Sep 11, 2015

Uh oh!

crosbymichael commented Sep 11, 2015

Uh oh!

mrunalp commented Sep 11, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants